Standardize MatterClient access via Lit context (#421) by markvp · Pull Request #599 · matter-js/matterjs-server

markvp · 2026-05-03T01:25:33Z

Summary

Introduces a ContextProvider in matter-dashboard-app that broadcasts the MatterClient instance to all descendant components via @lit/context
Converts all 17 dashboard components and dialogs from manual .client prop drilling to @consume({ context: clientContext, subscribe: true }) decorators
Removes the client argument from showCommissionNodeDialog(), showLogLevelDialog(), and showNodeBindingDialog() helpers — dialogs now receive the client from context automatically

Test plan

Manual validation performed against a live server instance (npm run server -- --storage-path data) with a diagnostics dump imported as test nodes:

Build and lint:

npm run format — passes
npm run lint — 0 errors, 0 warnings
npm run build — passes

Closes #421

🤖 Generated with Claude Code

Copilot

Pull request overview

Standardizes how the dashboard accesses the shared MatterClient by providing it via @lit/context from matter-dashboard-app, removing widespread .client prop drilling and simplifying dialog helper APIs.

Changes:

Add a clientContext provider in matter-dashboard-app and consume it across dashboard views/components.
Convert dialogs/components to @consume({ context: clientContext, subscribe: true }) and stop passing .client through templates.
Remove MatterClient parameters from dialog “show*Dialog” helpers and rely on context inside the dialogs.

Reviewed changes

Copilot reviewed 17 out of 17 changed files in this pull request and generated 2 comments.

Show a summary per file

File	Description
packages/dashboard/src/pages/network/update-connections-dialog.ts	Switch dialog to consume `MatterClient` from `clientContext` instead of a passed `.client` prop.
packages/dashboard/src/pages/network/network-details.ts	Stop passing `.client` into `<update-connections-dialog>`.
packages/dashboard/src/pages/matter-server-view.ts	Consume `MatterClient` from context; stop passing `.client` into children like header/server-details.
packages/dashboard/src/pages/matter-node-view.ts	Consume `MatterClient` from context; remove `.client` prop drilling to header/node-details.
packages/dashboard/src/pages/matter-network-view.ts	Consume `MatterClient` from context; remove `.client` prop drilling to header.
packages/dashboard/src/pages/matter-endpoint-view.ts	Consume `MatterClient` from context; remove `.client` prop drilling to node-details/header.
packages/dashboard/src/pages/matter-dashboard-app.ts	Remove `.client` passing into routed views, relying on context instead.
packages/dashboard/src/pages/matter-cluster-view.ts	Consume `MatterClient` from context and rely on context for cluster command panels.
packages/dashboard/src/pages/components/server-details.ts	Consume `MatterClient` from context; update commission/import flows to use context-based dialogs.
packages/dashboard/src/pages/components/node-details.ts	Consume `MatterClient` from context; update binding dialog invocation signature.
packages/dashboard/src/pages/components/header.ts	Consume `MatterClient` from context; open settings dialog without passing client.
packages/dashboard/src/pages/cluster-commands/base-cluster-commands.ts	Base class now consumes `MatterClient` from context for all cluster command panels.
packages/dashboard/src/components/dialogs/settings/show-log-level-dialog.ts	Remove client arg; create dialog that consumes client via context.
packages/dashboard/src/components/dialogs/settings/log-level-dialog.ts	Consume `MatterClient` from context inside dialog.
packages/dashboard/src/components/dialogs/commission-node-dialog/show-commission-node-dialog.ts	Remove client arg; create dialog that consumes client via context.
packages/dashboard/src/components/dialogs/commission-node-dialog/commission-node-dialog.ts	Consume `MatterClient` from context inside dialog.
packages/dashboard/src/components/dialogs/binding/show-node-binding-dialog.ts	Remove client arg; create dialog that consumes client via context.

markvp · 2026-05-03T06:31:18Z

fixes #421

Replace manual `.client` prop drilling across 17 dashboard components with a `ContextProvider` in `matter-dashboard-app` and `@consume` decorators in all consumers, so components receive the client automatically without explicit parent-to-child bindings. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>

provider.setValue is now called immediately after startListening resolves so consumers receive a non-undefined client before the first nodes_changed event. network-details was the only consumer missing subscribe: true, leaving it with a stale client if the provider value changed after initial render. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>

Copilot

Pull request overview

Copilot reviewed 17 out of 17 changed files in this pull request and generated 1 comment.

        this.client.startListening().then(
            () => {
                this._state = "connected";
+                this.provider.setValue(clone(this.client));
                this._setupEventListeners();


Apollon77

Review summary

Great direction — context replaces the prop-drilling chain cleanly. Build/lint pass. Two correctness concerns and several consistency follow-ups before merge.

🔴 Critical

Clone-induced msgId divergence → silent command collisions (see inline on matter-dashboard-app.ts:111)

🟡 Important

Settings dialog chain not migrated — partial standardization (inline on header.ts)
Leftover .client= bindings on now-consuming children (inline on cluster/endpoint/node-view + network-details)
network-details.ts subscribe: false → true not justified by code reads (inline)
Type lies: client?: MatterClient → client!: MatterClient paired with runtime if (this.client) guards (inline on header + server-details)

⚪ Nits (no in-diff anchor)

1. ContextProvider(this, { context: clientContext, initialValue: this.client }) (matter-dashboard-app.ts:51) — field initializer runs during element construction; dashboard.client = client in entrypoint/main.ts runs after construction. So initialValue is always undefined. Reads as if seeding a value; doesn't. Drop initialValue or move provider creation into a setter/firstUpdated after assignment.

2. @consume({subscribe:true}) @property({attribute:false}) stacking — @property is technically redundant; @consume already triggers requestUpdate on subscribed value changes. Harmless but noise. Inconsistent across files (some have it, matter-cluster-view.ts etc. didn't originally). Pick one and unify.

3. packages/dashboard/src/util/clone_class.ts (not in diff but now load-bearing for context propagation):

Uses any and lacks return type — violates CLAUDE.md "avoid type casts / use generics":

export const clone = <T extends object>(orig: T): T =>
    Object.assign(Object.create(Object.getPrototypeOf(orig)), orig);

Shallow Object.assign skips ES private fields (#x), accessor-backed state, and non-enumerable own properties. If MatterClient ever grows a #field, clone silently loses it.
If you fix the critical msgId concern by removing the clone-on-publish pattern (recommended), this file may become unused — preferable.

The msgId issue is the only blocker — others are polish to deliver the "standardize" promise the title makes.

Apollon77 · 2026-05-04T07:46:55Z

        this.client.startListening().then(
            () => {
                this._state = "connected";
+                this.provider.setValue(clone(this.client));


🔴 Critical: clone(this.client) causes msgId divergence → silent command collisions.

MatterClient.msgId is a TS-private primitive (packages/ws-client/src/client.ts:55) incremented by sendCommand (client.ts:371). result_futures (client.ts:46) is a shared object reference. The shallow Object.assign(Object.create(proto), orig) in util/clone_class.ts copies msgId by value but shares result_futures by reference.

Failure path:

Original sends startListening → original.msgId = V.

setValue(clone-A) published; clone-A.msgId = V. UI sends command → clone-A.msgId = V+1, writes result_futures["V+1"] = {clone-A's resolve}.

nodes_changed fires (frequent — every attribute storm) → setValue(clone-B) made from original whose msgId is still V → clone-B.msgId = V.

UI sends another command via clone-B → clone-B.msgId = V+1, writes result_futures["V+1"] = {clone-B's resolve}, overwriting clone-A's pending entry.

Server reply for the first command resolves clone-B's promise. Clone-A's promise hangs until commandTimeout (default 5 minutes) and rejects with CommandTimeoutError.

Silent, intermittent, scales with UI activity during attribute updates.

Fix: don't snapshot the client. Keep the context value as the live MatterClient ref and trigger consumer re-renders via a separate signal — e.g. a tickContext that increments, a custom event on the host, or a Lit reactive controller. Same applies to the two setValue(clone(...)) calls in _setupEventListeners.

Fixed — and updated further after reflection.

Initial fix replaced clone(this.client) with provider.setValue(this.client, true), which solved the identity problem but used force: true as an escape hatch rather than a proper design.

On review, your suggested approach is the right one. Switched to a dedicated tickContext:

clientContext is set once after startListening() resolves — a stable reference that never changes, so no force or subscription needed.

tickContext is a plain incrementing number, published on every nodes_changed / server_info_updated event. Consumers subscribe to this for re-renders.

The two concerns are now explicitly separated: stable ref for data, tick for change signal.

clone_class.ts is deleted. settings-dialog and log-level-dialog drop their client consume entirely — neither uses this.client directly after log-level-section migrated to context.

Apollon77 · 2026-05-04T07:46:55Z

-    public client?: MatterClient;
+    @consume({ context: clientContext, subscribe: true })
+    @property({ attribute: false })
+    public client!: MatterClient;


🟡 Inconsistent migration — settings dialog chain still prop-drills.

_openSettings (line 74) still calls showSettingsDialog(this.client). Inside that chain:

show-settings-dialog.ts:9-12 — receives client, assigns dialog.client = client

settings-dialog.ts:23 — bare @property (no @consume)

settings-dialog.ts:92 — passes .client=${this.client} to <log-level-section>

log-level-section.ts — bare @property

Sibling dialogs (commission/log-level/binding) were migrated. Either finish the settings chain for consistency or scope-clarify in the PR description.

Also a type-lie nit on this line: client!: MatterClient (non-null assertion) but lines 73, 165, 178, 190 all guard with if (this.client) — runtime admits undefined. Original client?: MatterClient was more honest. Either keep ?: or remove the guards.

Both addressed.

Settings chain: showSettingsDialog now takes no arguments. settings-dialog and log-level-section both use @consume({ context: clientContext, subscribe: true }). The dialog is appended to matter-dashboard-app's renderRoot, so it sits inside the provider's shadow tree and receives context naturally — consistent with how commission/log-level/binding dialogs were already handled.

Type lie: client!: MatterClient → client?: MatterClient, @property({ attribute: false }) removed (see nit below on @consume + @property stacking).

Apollon77 · 2026-05-04T07:46:55Z

-    public client?: MatterClient;
+    @consume({ context: clientContext, subscribe: true })
+    @property({ attribute: false })
+    public client!: MatterClient;


Type lie: client!: MatterClient paired with if (!this.client) return html\`;on line 32. The runtime guard says it can beundefined; the !says it can't. Originalclient?: MatterClientwas correct. Pick a stance — the codebase is inconsistent (this file asserts,header.ts` guards).

Fixed: client!: MatterClient → client?: MatterClient, @property({ attribute: false }) removed. The one access site not covered by the existing if (!this.client) return html``` guard — the importTestNodecall inside areader.onload` closure — was updated to use optional chaining.

Apollon77 · 2026-05-04T07:46:55Z

@@ -130,12 +133,11 @@ class MatterClusterView extends LitElement {
            <dashboard-header
                .title=${`Node ${this.node.node_id} ${nodeHex}  |  Endpoint ${this.endpoint}  |  Cluster ${this.cluster} (${clusterName})`}


Leftover .client= binding to flag — line 114 (the "Not found" branch) still passes .client=${this.client} to <dashboard-header>, even though <dashboard-header> now consumes from context. The success branch was cleaned up here; the not-found branch was missed. Drop for consistency.

Apollon77 · 2026-05-04T07:46:55Z

                .title=${`Node ${this.node.node_id} ${nodeHex}  |  Endpoint ${this.endpoint}`}
                .backButton=${`#node/${this.node.node_id}`}
-                .client=${this.client}
            ></dashboard-header>


Leftover .client= binding — line 66 ("Not found" branch) still passes .client=${this.client} to <dashboard-header>. Success branch cleaned, not-found missed. Drop.

Apollon77 · 2026-05-04T07:46:55Z

-                .client=${this.client}
-                backButton="#"
-            ></dashboard-header>
+            <dashboard-header .title=${`Node ${this.node.node_id} ${nodeHex}`} backButton="#"></dashboard-header>


Leftover .client= binding — line 53 ("Node not found" branch) still passes .client=${this.client} to <dashboard-header>. Drop for consistency with the success branch.

Apollon77 · 2026-05-04T07:46:55Z

    public threadEdgePairs: Map<string, ThreadEdgePair> = new Map();

-    @consume({ context: clientContext })
+    @consume({ context: clientContext, subscribe: true })


🟡 subscribe: false → true is not justified by code reads in this file. Only client access is this.client?.serverInfo?.fabric_index (line 124). serverInfo is a getter that reads through to the shared connection.serverInfo object, mutated in-place on server_info_updated. All other reactive data flows in via props (nodes, borderRouters, etc.). Subscribing here only adds redundant re-renders on every nodes_changed (frequent). Revert to subscribe: false unless there's a specific case I'm missing.

Also: lines 1084 and ~1162 — there are still leftover .client=${this.client} bindings on <update-connections-dialog> in some branches even though that child now consumes from context. Drop them.

Both addressed.

subscribe: reverted to subscribe: false (the default, so the option is just dropped). You're right — serverInfo is a getter over a mutated-in-place object and all other reactive data flows in via props; no subscription is warranted here.

Leftover .client=: the binding at line 1084 on <update-connections-dialog> in the border-router branch is removed. The instance at line ~1161 (node branch) never had it — that one was already clean.

markvp · 2026-05-04T08:11:37Z

Addressing the review body nits:

1. initialValue: this.client
Dropped. The field initialiser runs during element construction before dashboard.client = client is assigned, so the value was always undefined. The provider is now constructed with just { context: clientContext }.

2. @consume + @property stacking
Removed the redundant @property({ attribute: false }) from every @consume-decorated client field across all consumer files. @consume({ subscribe: true }) already calls requestUpdate() on value changes; the @property was pure noise. Unified on @consume-only throughout.

3. clone_class.ts
Deleted — unused once the clone-on-publish pattern was removed.

Copilot AI review requested due to automatic review settings May 3, 2026 01:25

Copilot started reviewing on behalf of markvp May 3, 2026 01:25 View session

Copilot AI reviewed May 3, 2026

View reviewed changes

Comment thread packages/dashboard/src/pages/matter-dashboard-app.ts

Comment thread packages/dashboard/src/pages/network/network-details.ts

markvp and others added 2 commits May 3, 2026 00:49

markvp force-pushed the feat/421-standardize-client-context branch from 264b4c9 to dd7d943 Compare May 3, 2026 07:50

Apollon77 requested a review from Copilot May 4, 2026 07:32

Copilot started reviewing on behalf of Apollon77 May 4, 2026 07:33 View session

Copilot AI reviewed May 4, 2026

View reviewed changes

Comment thread packages/dashboard/src/pages/matter-dashboard-app.ts

Comment on lines 108 to 112

this.client.startListening().then(

() => {

this._state = "connected";

this.provider.setValue(clone(this.client));

this._setupEventListeners();

Apollon77 reviewed May 4, 2026

View reviewed changes

		@@ -130,12 +133,11 @@ class MatterClusterView extends LitElement {
		<dashboard-header
		.title=${`Node ${this.node.node_id} ${nodeHex} \| Endpoint ${this.endpoint} \| Cluster ${this.cluster} (${clusterName})`}

Conversation

markvp commented May 3, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Summary

Test plan

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Reviewed changes

Uh oh!

Uh oh!

Uh oh!

markvp commented May 3, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Uh oh!

Apollon77 left a comment

Choose a reason for hiding this comment

Review summary

🔴 Critical

🟡 Important

⚪ Nits (no in-diff anchor)

Uh oh!

Choose a reason for hiding this comment

Uh oh!

markvp May 4, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

markvp commented May 4, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

markvp commented May 3, 2026 •

edited

Loading

markvp commented May 3, 2026 •

edited

Loading

markvp May 4, 2026 •

edited

Loading